Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[java] Ensure retry mechanism does not swallow an exception #12838

Merged
merged 10 commits into from
Oct 6, 2023

Conversation

pujagani
Copy link
Contributor

Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

Ensure retry mechanism does not swallow an exception

Motivation and Context

Related to #12558 (comment).

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@pujagani pujagani marked this pull request as draft September 29, 2023 14:49
Copy link
Member

@shs96c shs96c left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One comment, but looks great. Love the tests.

private static final Fallback<Object> fallback =
Fallback.of(
executionAttemptedEvent -> {
if (executionAttemptedEvent.getLastException() != null) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given the length of this, I'd suggest pulling out it out into a private method in this class. It'll make it easier to visually figure out the fields of the class from the methods it contains.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh makes sense. Thank you! Will do that.

@codecov-commenter
Copy link

codecov-commenter commented Oct 3, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (b9bdff1) 56.51% compared to head (56fbeb5) 56.51%.
Report is 3 commits behind head on trunk.

❗ Current head 56fbeb5 differs from pull request most recent head dbe8791. Consider uploading reports for the commit dbe8791 to get more accurate results

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff           @@
##            trunk   #12838   +/-   ##
=======================================
  Coverage   56.51%   56.51%           
=======================================
  Files          86       86           
  Lines        5255     5255           
  Branches      187      187           
=======================================
  Hits         2970     2970           
  Misses       2098     2098           
  Partials      187      187           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pujagani pujagani marked this pull request as ready for review October 3, 2023 11:40
@pujagani pujagani added the C-java label Oct 3, 2023
@titusfortner
Copy link
Member

Does this fix #12558 or just related to the discussion there?

@titusfortner titusfortner added this to the 4.14 milestone Oct 5, 2023
@diemol
Copy link
Member

diemol commented Oct 5, 2023

#12558 was fixed via 5f3f7d0, but the issue diverted to another error which is getting fixed with this PR.

@pujagani pujagani merged commit a67b81d into SeleniumHQ:trunk Oct 6, 2023
4 checks passed
@joerg1985
Copy link
Member

@pujagani thanks for fixing this 😀

aguspe pushed a commit to aguspe/selenium that referenced this pull request Oct 22, 2023
aguspe pushed a commit to aguspe/selenium that referenced this pull request Oct 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants